Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

async/await: improve not-send errors #64895

Merged
merged 1 commit into from
Oct 1, 2019
Merged

async/await: improve not-send errors #64895

merged 1 commit into from
Oct 1, 2019

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Sep 29, 2019 •
edited by nikomatsakis
Loading

cc #64130.

note: future does not implement `std::marker::Send` because this value is used across an await
  --> $DIR/issue-64130-non-send-future-diags.rs:15:5
   |
LL |     let g = x.lock().unwrap();
   |         - has type `std::sync::MutexGuard<'_, u32>`
LL |     baz().await;
   |     ^^^^^^^^^^^ await occurs here, with `g` maybe used later
LL | }
   | - `g` is later dropped here

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@davidtwco davidtwco force-pushed the issue-64130-async-error-definition branch from 42ce06e to f7cb10d Compare September 29, 2019 17:36
Centril
Centril reviewed Sep 29, 2019
View reviewed changes
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some nits.

src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

Sign in to view
@davidtwco davidtwco force-pushed the issue-64130-async-error-definition branch from f7cb10d to bdaec6a Compare September 29, 2019 19:18
@davidtwco
Copy link
Member Author

I'm not sure where the duplicate diagnostic item error is coming from, a quick glance only found the rustc_diagnostic_item attribute that I added.

@Centril
Copy link
Contributor

Centril commented Sep 29, 2019

cc @oli-obk ^---

@rust-highfive

This comment has been minimized.

Sign in to view
oli-obk
oli-obk reviewed Sep 30, 2019
View reviewed changes
src/libstd/future.rs Outdated Show resolved Hide resolved
@davidtwco davidtwco force-pushed the issue-64130-async-error-definition branch from bdaec6a to e0f08b5 Compare September 30, 2019 09:13
@davidtwco davidtwco force-pushed the issue-64130-async-error-definition branch from e0f08b5 to 28be5ae Compare September 30, 2019 18:29
@oli-obk
Copy link
Contributor

oli-obk commented Sep 30, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Sep 30, 2019

📌 Commit 28be5ae8c558613e59a56eb83f069da47eda56ed has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2019
nikomatsakis
nikomatsakis requested changes Sep 30, 2019
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome. Beautiful work. My one concern is that I think we can do better still with the error, but I'm sort of inclined to land this PR and continue that work afterwards.

src/test/ui/async-await/issue-64130-non-send-future-diags.stderr Show resolved Hide resolved
src/librustc/traits/error_reporting.rs Show resolved Hide resolved
src/librustc_typeck/check/generator_interior.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/generator_interior.rs Outdated Show resolved Hide resolved
@davidtwco davidtwco force-pushed the issue-64130-async-error-definition branch from 28be5ae to 9076646 Compare September 30, 2019 22:15
nikomatsakis
nikomatsakis requested changes Sep 30, 2019
View reviewed changes
src/librustc/ty/context.rs Outdated Show resolved Hide resolved
src/librustc/ty/context.rs Outdated Show resolved Hide resolved
@davidtwco
async/await: improve obligation errors
04fa9b1 
This commit improves obligation errors for async/await:

```
note: future does not implement `std::marker::Send` because this value is used across an
      await
  --> $DIR/issue-64130-non-send-future-diags.rs:15:5
   |
LL |     let g = x.lock().unwrap();
   |         - has type `std::sync::MutexGuard<'_, u32>`
LL |     baz().await;
   |     ^^^^^^^^^^^ await occurs here, with `g` maybe used later
LL | }
   | - `g` is later dropped here
```

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco davidtwco force-pushed the issue-64130-async-error-definition branch from 9076646 to 04fa9b1 Compare September 30, 2019 22:41
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 30, 2019

📌 Commit 04fa9b1 has been approved by nikomatsakis

Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
@Centril
Rollup merge of rust-lang#64895 - davidtwco:issue-64130-async-error-d…
25f774a 
…efinition, r=nikomatsakis

async/await: improve not-send errors

cc rust-lang#64130.

```
note: future does not implement `std::marker::Send` because this value is used across an await
  --> $DIR/issue-64130-non-send-future-diags.rs:15:5
   |
LL |     let g = x.lock().unwrap();
   |         - has type `std::sync::MutexGuard<'_, u32>`
LL |     baz().await;
   |     ^^^^^^^^^^^ await occurs here, with `g` maybe used later
LL | }
   | - `g` is later dropped here
```

r? @nikomatsakis
@Centril Centril mentioned this pull request Oct 1, 2019
Merged
bors added a commit that referenced this pull request Oct 1, 2019
@bors
Auto merge of #64946 - Centril:rollup-66mj5o0, r=Centril
702b45e 
Rollup of 10 pull requests

Successful merges:

 - #63674 (syntax: Support modern attribute syntax in the `meta` matcher)
 - #63931 (Stabilize macros in some more positions)
 - #64887 (syntax: recover trailing `|` in or-patterns)
 - #64895 (async/await: improve not-send errors)
 - #64896 (Remove legacy grammar)
 - #64907 (A small amount of tidying-up factored out from PR #64648)
 - #64928 (Add tests for some issues)
 - #64930 (Silence unreachable code lint from await desugaring)
 - #64935 (Improve code clarity)
 - #64937 (Deduplicate closure type errors)

Failed merges:

r? @ghost
@bors bors merged commit 04fa9b1 into rust-lang:master Oct 1, 2019
@bors
Copy link
Contributor

bors commented Oct 1, 2019

☔ The latest upstream changes (presumably #64946) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 1, 2019
@davidtwco davidtwco deleted the issue-64130-async-error-definition branch October 1, 2019 15:32
@sinkuu sinkuu mentioned this pull request Oct 2, 2019
Closed
@ghost
Copy link

ghost commented Oct 2, 2019 •
edited by ghost
Loading

It appears that this causes ICE #64964 (comment)

I'm currently retesting that by applying this PR on top of the rust compilation that succeeded without it.confirmed

@nikomatsakis nikomatsakis added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 3, 2019
@nikomatsakis
Copy link
Contributor

I'd like to nominate this PR (and the follow-up #64989) for inclusion in beta. It's not the sort of thing I would normally include, but this error is (a) one of the most common problems I hear people complaining about with async-await and (b) particularly opaque without this fix. Moreover, it is early in the cycle, so it will presumably get a reasonable amount of testing before landing. The change is relatively low risk, in that it is confined to error reporting.

@nikomatsakis nikomatsakis mentioned this pull request Oct 3, 2019
Merged
@Centril Centril added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 3, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Oct 3, 2019

discussed at rustc meeting. accepted for beta backport in tandem with PR #64989 (since that fixes an ICE this injected).

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 3, 2019
@davidtwco davidtwco mentioned this pull request Oct 12, 2019
Merged
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 22, 2019
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Oct 22, 2019
Merged
bors added a commit that referenced this pull request Oct 24, 2019
@bors
Auto merge of #65708 - Mark-Simulacrum:beta-backports, r=pietroalbini
def6df2 
[beta] backport rollup

This includes a bunch of PRs:
 *  Fix redundant semicolon lint interaction with proc macro attributes #64387
 *  Upgrade async/await to "used" keywords. #64875
 *  syntax: fix dropping of attribute on first param of non-method assocated fn #64894
 *  async/await: improve not-send errors #64895
 *  Silence unreachable code lint from await desugaring #64930
 *  Always mark rust and rust-call abi's as unwind #65020
 *  Account for macro invocation in `let mut $pat` diagnostic. #65123
 *  Ensure that associated `async fn`s have unique fresh param names #65142
 *  Add troubleshooting section to PGO chapter in rustc book. #65402
 *  Upgrade GCC to 8.3.0, glibc to 1.17.0 and crosstool-ng to 1.24.0 for dist-armv7-linux #65302
 *  Optimize `try_expand_impl_trait_type` #65293
 *  use precalculated dominators in explain_borrow #65172
 *  Fix ICE #64964 #64989
bors added a commit that referenced this pull request Oct 26, 2019
@bors
Auto merge of #65708 - Mark-Simulacrum:beta-backports, r=pietroalbini
23f8f65 
[beta] backport rollup

This includes a bunch of PRs:
 *  Fix redundant semicolon lint interaction with proc macro attributes #64387
 *  Upgrade async/await to "used" keywords. #64875
 *  syntax: fix dropping of attribute on first param of non-method assocated fn #64894
 *  async/await: improve not-send errors #64895
 *  Silence unreachable code lint from await desugaring #64930
 *  Always mark rust and rust-call abi's as unwind #65020
 *  Account for macro invocation in `let mut $pat` diagnostic. #65123
 *  Ensure that associated `async fn`s have unique fresh param names #65142
 *  Add troubleshooting section to PGO chapter in rustc book. #65402
 *  Upgrade GCC to 8.3.0, glibc to 1.17.0 and crosstool-ng to 1.24.0 for dist-armv7-linux #65302
 *  Optimize `try_expand_impl_trait_type` #65293
 *  use precalculated dominators in explain_borrow #65172
 *  Fix ICE #64964 #64989
 *  [beta] Revert "Auto merge of #62948 - matklad:failable-file-loading, r=petro… #65273
 *  save-analysis: Don't ICE when resolving qualified type paths in struct members #65353
 *  save-analysis: Nest tables when processing impl block definitions #65511
 *  Avoid ICE when checking `Destination` of `break` inside a closure #65518
 *  Avoid ICE when adjusting bad self ty #65755
 *  workaround msys2 bug #65762
bors added a commit that referenced this pull request Dec 11, 2019
@bors
Auto merge of #65345 - davidtwco:issue-64130-async-send-sync-error-im…
27d6f55 
…provements, r=nikomatsakis

async/await: improve not-send errors, part 2

Part of #64130. Fixes #65667.

This PR improves the errors introduced in #64895 so that they have specialized messages for `Send` and `Sync`.

r? @nikomatsakis
@tmandry tmandry mentioned this pull request Dec 17, 2019
Closed
@eddyb eddyb mentioned this pull request Apr 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@Centril Centril Centril left review comments

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@davidtwco @rust-highfive @Centril @oli-obk @bors @nikomatsakis @pnkfelix @Mark-Simulacrum